Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: better error and loading handling on programs page #1012

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dhess
Copy link
Member

@dhess dhess commented Jul 10, 2023

No description provided.

@dhess
Copy link
Member Author

dhess commented Jul 12, 2023

Unless I'm misunderstanding something, I'm afraid we may need to disable exactOptionalPropertyTypes. I would like to use Adobe's react-aria package for our "headless" components, and it seems to have some issues with this TypeScript setting:

https://github.com/hackworthltd/primer-app/actions/runs/5537221041/jobs/10105813957?pr=1012#step:7:17

It's not enabled in the upstream project, either:

https://github.com/adobe/react-spectrum/blob/main/tsconfig.json

@georgefst
Copy link
Contributor

Unless I'm misunderstanding something, I'm afraid we may need to disable exactOptionalPropertyTypes. I would like to use Adobe's react-aria package for our "headless" components, and it seems to have some issues with this TypeScript setting:

I would have assumed that the compiler options we set for our package are not passed to dependencies, since the creators of a particular library may intentionally ignore particular options. Is this not the case?

Anyway, I wouldn't actually mind too much about disabling this particular option. It can be quite annoying differentiating between x:? T and x: T | undefined, and we have boilerplate code in several places for dealing with this. And while they aren't technically equivalent at runtime, I think since we embrace strong types in our codebase, we don't tend to write the sort of code that would distinguish between them.

@dhess
Copy link
Member Author

dhess commented Jul 13, 2023

Unless I'm misunderstanding something, I'm afraid we may need to disable exactOptionalPropertyTypes. I would like to use Adobe's react-aria package for our "headless" components, and it seems to have some issues with this TypeScript setting:

I would have assumed that the compiler options we set for our package are not passed to dependencies, since the creators of a particular library may intentionally ignore particular options. Is this not the case?

I'm also confused about that, but it appears not. Maybe I've misconfigured something.

Anyway, I wouldn't actually mind too much about disabling this particular option. It can be quite annoying differentiating between x:? T and x: T | undefined, and we have boilerplate code in several places for dealing with this. And while they aren't technically equivalent at runtime, I think since we embrace strong types in our codebase, we don't tend to write the sort of code that would distinguish between them.

I feel similarly, so I'll just disable it in this PR, then.

@dhess
Copy link
Member Author

dhess commented Jul 13, 2023

If I enable this option:

"skipLibCheck": false,

then the problem goes away. It's documented here. It seems like this alternative solution is preferred to skipping the lib check, however.

@dhess
Copy link
Member Author

dhess commented Jul 13, 2023

I've tried a few different lib overrides like ES2017.Intl etc., and they all have the same issue, or create more issues. The upstream library targets es6 but specifies esnext in its lib, so it's not clear what they actually expect downstream users to choose:

https://github.com/adobe/react-spectrum/blob/8c9d3bdc279568e76e406adc924607c04d796009/tsconfig.json#L6
https://github.com/adobe/react-spectrum/blob/8c9d3bdc279568e76e406adc924607c04d796009/tsconfig.json#L26

So I think I'll just disable exactOptionalPropertyTypes in our project and move on.

The error and loading messages are just placeholders for now. The
crucial change here is to let the student know when we can't display
up-to-date session information for some reason.

Note that we show data if it's available, without first checking for
`isLoading` or `isError`. This means we may show stale data, but we prefer
this over showing a loading message or an error for short server
outages. For some justification of this approach, see:

https://tkdodo.eu/blog/status-checks-in-react-query

Note that React Query will not show stale data indefinitely, and will
eventually show an error message if the data is stale for too long.

Signed-off-by: Drew Hess <[email protected]>
The previous commit improved matters on the session page by handling
`isError` and `isLoading` conditions, but has annoying behavior when
the student is typing a program name into the search bar and we're
doing live search, because the sessions page was rendered
all-or-nothing.

Now we always show the nav bar. This means we no longer have a nice
single component for the sessions page (i.e., the `SessionsPage`
component is no more), but `ChooseSession` is now more or less the
same thing, only with better error and loading behavior. I haven't
bothered to add a Storybook story for it, but it would be easyenough
to add later if we need it.

Signed-off-by: Drew Hess <[email protected]>
This setting is causing problems with one of `react-aria`'s
dependencies, and we've found it to be probably more annoying than
it's worth anyway, on balance.

Signed-off-by: Drew Hess <[email protected]>
dhess added a commit that referenced this pull request Apr 10, 2024
This setting is causing problems for us with one package we'd like to
use:

#1012 (comment)

and is also preventing us from migrating to TypeScript 5 due to an
upstream issue in `react-router` that's been unaddressed for almost a
year:

#898

There were no objections to disabling it, so we do so in this commit.
(There is one minor code change required as a result, just an
additional optional property check on a `onNodeClick` handler in our
React Flow wrapper.)

Signed-off-by: Drew Hess <[email protected]>
dhess added a commit that referenced this pull request Apr 10, 2024
This setting is causing problems for us with one package we'd like to
use:

#1012 (comment)

and is also preventing us from migrating to TypeScript 5 due to an
upstream issue in `react-router` that's been unaddressed for almost a
year:

#898

There were no objections to disabling it, so we do so in this commit.
(There is one minor code change required as a result, just an
additional optional property check on a `onNodeClick` handler in our
React Flow wrapper.)

Closes #898

Signed-off-by: Drew Hess <[email protected]>
@dhess
Copy link
Member Author

dhess commented Apr 10, 2024

I've disabled exactOptionalPropertyTypes in #1150, so I'll come back to this PR soon-ish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants